Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename teal_data_module.R functions #1430

Merged
merged 21 commits into from
Dec 19, 2024
Merged

Rename teal_data_module.R functions #1430

merged 21 commits into from
Dec 19, 2024

Conversation

llrs-roche
Copy link
Contributor

@llrs-roche llrs-roche commented Dec 13, 2024 •
edited
Loading

Pull Request

  • srv_teal_data() & ui_teal_data(): renamed to srv_teal_data_module() & ui_teal_data_module().
  • inside srv_teal_data_module():
    • renamed data to teal_data_r (teal_data reactive ): This suffix is what is used inside tmc and tmg modules.
    • rename data_module to teal_data_module.
    • rename data_out into module_out (to keep it separate from data but close to teal_data_module)
  • inside srv_validate_reactive_teal_data(): rename data argument into teal_data_r to match previous renaming and schema in the framework.
  • inside srv_validate_reactive_teal_data(): The tryCatch code moved to srv_teal_data_module and as the code checked is module_out I renamed the output to try_module_out.
  • inside .fallback_on_failure(): There is no longer .fallback_on_failure().

Fixes #1323

Due to the renaming I had to change some tests.

I checked interactively the examples with devtools::run_examples() as well as with R CMD check. I'm not sure whether this PR broke the examples on ?teal::teal_slices. This is an internal function with some examples that only display the ID, and report there is no matching id.

@llrs-roche llrs-roche mentioned this pull request Dec 13, 2024
Closed
m7pr
m7pr reviewed Dec 16, 2024
View reviewed changes
R/module_teal_data.R Outdated Show resolved Hide resolved
m7pr
m7pr reviewed Dec 16, 2024
View reviewed changes
R/module_teal_data.R Outdated Show resolved Hide resolved
@llrs-roche
Copy link
Contributor Author

Renaming srv_validate_reactive_teal_data data to something else have some code implications, as even if it is an internal function the documentation is then reused on some exported function (srv_transform_teal_data).

I can rename the external function parameter to match the new one but this will require renaming multiple packages and modules (notably the new srv_decorate_teal_data function introduced on teal.modules.general (tmg) and teal.modules.clinical (tmc)). I am not sure how many packages could be affected by this renaming.

I could also duplicate the documentation on the internal function srv_validate_reactive_teal_data with the renamed parameter and keep using the old parameter on srv_transform_teal_data with the documentation for that parameter copied on its documentation file.

Renaming {ui,srv}_teal_data to {ui,srv}_teal_data_module was made via an alias, so I don't expect any function to break due to this change. But it might be worth to check how this renaming would affect teal

Pushing to see checks results but I expect them to fail, as the path I took was to rename the parameter name but haven't changed anything related to it on tests (or dependencies).

@llrs-roche llrs-roche marked this pull request as ready for review December 17, 2024 10:03
@m7pr m7pr self-assigned this Dec 17, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 17, 2024 •
edited
Loading

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------
R/checkmate.R                        24       0  100.00%
R/dummy_functions.R                  67      11  83.58%   41, 43, 85-93
R/get_rcode_utils.R                  12       0  100.00%
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                             99      42  57.58%   150-159, 161, 173-194, 219-222, 229-235, 238-239, 241
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_bookmark_manager.R         158     127  19.62%   47-68, 88-138, 143-144, 156, 203, 238-315
R/module_data_summary.R             203      37  81.77%   26-54, 68, 78, 232, 263-267
R/module_filter_data.R               64       2  96.88%   22-23
R/module_filter_manager.R           230      57  75.22%   56-62, 73-82, 90-95, 108-112, 117-118, 291-314, 340, 367, 379, 386-387
R/module_init_data.R                 74       0  100.00%
R/module_nested_tabs.R              227      85  62.56%   40-136, 168, 193-195, 312, 346
R/module_snapshot_manager.R         216     146  32.41%   89-95, 104-113, 121-133, 152-153, 170-180, 184-199, 201-208, 215-230, 234-238, 240-246, 249-262, 265-273, 303-317, 320-331, 334-340, 354
R/module_teal_data.R                149      76  48.99%   44-150
R/module_teal_lockfile.R            131      44  66.41%   32-36, 44-56, 59-61, 75, 85-87, 99-101, 109-118, 121, 123, 125-126, 160-161
R/module_teal_with_splash.R          12      12  0.00%    22-38
R/module_teal.R                     195      87  55.38%   48-143, 158, 184-185, 224
R/module_transform_data.R           110       4  96.36%   20, 59, 129-130
R/modules.R                         278      71  74.46%   171-175, 230-233, 354-374, 382, 532-538, 551-559, 574-622, 655, 667-675
R/reporter_previewer_module.R        19       2  89.47%   30, 34
R/show_rcode_modal.R                 24      24  0.00%    17-42
R/tdata.R                            14      14  0.00%    19-61
R/teal_data_module-eval_code.R       24       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                 20       0  100.00%
R/teal_data_utils.R                  10       0  100.00%
R/teal_reporter.R                    68       6  91.18%   69, 77, 125-126, 129, 146
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      63       0  100.00%
R/teal_transform_module.R            45       0  100.00%
R/TealAppDriver.R                   353     353  0.00%    52-735
R/utils.R                           253      38  84.98%   405-454
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              15      11  26.67%   4-18
TOTAL                              3330    1328  60.12%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  -------
R/module_nested_tabs.R       +1       0  +0.17%
TOTAL                        +1       0  +0.01%

Results for commit: 9d2e771

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 17, 2024 •
edited
Loading

Unit Tests Summary

  1 files   27 suites   10m 27s ⏱️
275 tests 257 ✅ 18 💤 0 ❌
501 runs  483 ✅ 18 💤 0 ❌

Results for commit 9d2e771.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 17, 2024 •
edited
Loading

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
module_teal 💔 $149.42$ $+2.91$ $0$ $0$ $0$ $0$
shinytest2-data_summary 💚 $49.73$ $-49.67$ $-1$ $+6$ $0$ $0$
shinytest2-decorators 💚 $21.83$ $-21.79$ $-7$ $+2$ $0$ $0$
shinytest2-filter_panel 💚 $41.58$ $-41.54$ $-2$ $+3$ $0$ $0$
shinytest2-init 💚 $26.58$ $-26.54$ $-14$ $+3$ $0$ $0$
shinytest2-landing_popup 💚 $43.43$ $-43.37$ $-6$ $+5$ $0$ $0$
shinytest2-module_bookmark_manager 💚 $34.96$ $-34.91$ $0$ $+4$ $0$ $0$
shinytest2-modules 💚 $38.13$ $-38.07$ $-1$ $+4$ $0$ $0$
shinytest2-reporter 💚 $66.30$ $-66.24$ $-3$ $+5$ $0$ $0$
shinytest2-show-rcode 💚 $10.55$ $-10.54$ $-6$ $+1$ $0$ $0$
shinytest2-teal_data_module 💚 $47.66$ $-47.60$ $-2$ $+6$ $0$ $0$
shinytest2-teal_slices 💚 $61.19$ $-61.16$ $-16$ $+2$ $0$ $0$
shinytest2-utils 💚 $10.18$ $-10.17$ $-3$ $+1$ $0$ $0$
shinytest2-wunder_bar 💚 $21.97$ $-21.95$ $-2$ $+2$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
module_teal 💔 $18.38$ $+2.72$ creation_process_is_invoked_for_teal.lockfile.mode_enabled_and_snapshot_is_copied_to_teal_app.lock_and_removed_after_session_ended
shinytest2-data_summary 💚 $8.41$ $-8.40$ e2e_data_summary_just_list_the_unfilterable_objects_at_the_bottom_when_provided
shinytest2-data_summary 💚 $8.13$ $-8.12$ e2e_data_summary_table_displays_datasets_by_names_order_if_no_join_keys
shinytest2-data_summary 💚 $8.03$ $-8.02$ e2e_data_summary_table_displays_datasets_by_topological_sort_of_join_keys
shinytest2-data_summary 💚 $9.39$ $-9.38$ e2e_data_summary_table_does_not_list_unsupported_objects
shinytest2-data_summary 💚 $7.75$ $-7.74$ e2e_data_summary_table_is_displayed_with_2_columns_data_without_keys
shinytest2-data_summary 💚 $8.03$ $-8.02$ e2e_data_summary_table_is_displayed_with_3_columns_for_data_with_join_keys
shinytest2-decorators 💚 $11.22$ $-11.19$ e2e_module_with_decorator_UI_and_output_is_modified_interactively_upon_changes_in_decorator
shinytest2-decorators 💚 $10.61$ $-10.60$ e2e_module_with_decorator_where_server_fails_shows_shiny_error_message
shinytest2-filter_panel 💚 $15.57$ $-15.56$ e2e_filtering_a_module_specific_filter_is_not_refected_in_other_unshared_modules
shinytest2-filter_panel 💚 $15.93$ $-15.92$ e2e_filtering_a_module_specific_filter_is_reflected_in_other_shared_module
shinytest2-filter_panel 💚 $10.07$ $-10.06$ e2e_module_content_is_updated_when_data_is_filtered_in_filter_panel
shinytest2-init 💚 $8.25$ $-8.24$ e2e_init_creates_UI_containing_specified_title_favicon_header_and_footer
shinytest2-init 💚 $7.99$ $-7.98$ e2e_teal_app_initializes_with_no_errors
shinytest2-init 💚 $10.34$ $-10.33$ e2e_teal_app_initializes_with_sessionInfo_modal
shinytest2-landing_popup 💚 $8.29$ $-8.28$ e2e_app_with_customized_landing_popup_module_creates_modal_containing_specified_title_content_and_buttons
shinytest2-landing_popup 💚 $8.13$ $-8.12$ e2e_app_with_default_landing_popup_module_creates_modal_containing_a_button
shinytest2-landing_popup 💚 $8.16$ $-8.15$ e2e_teal_app_with_landing_popup_module_initializes_with_no_errors
shinytest2-landing_popup 💚 $8.02$ $-8.01$ e2e_when_customized_button_in_landing_popup_module_is_clicked_it_redirects_to_a_certain_page
shinytest2-landing_popup 💚 $10.83$ $-10.82$ e2e_when_default_landing_popup_module_is_closed_it_shows_the_underlying_teal_app
shinytest2-module_bookmark_manager 💚 $8.06$ $-8.05$ bookmark_manager_button_is_not_rendered_by_default
shinytest2-module_bookmark_manager 💚 $8.28$ $-8.27$ bookmark_manager_button_is_not_rendered_when_enableBookmarking_url_
shinytest2-module_bookmark_manager 💚 $8.24$ $-8.23$ bookmark_manager_button_is_rendered_when_enableBookmarking_server_
shinytest2-module_bookmark_manager 💚 $10.39$ $-10.37$ bookmark_manager_button_shows_modal_with_url_containing_state_id_when_clicked
shinytest2-modules 💚 $8.19$ $-8.18$ e2e_all_the_nested_teal_modules_are_initiated_as_expected
shinytest2-modules 💚 $9.97$ $-9.96$ e2e_filter_panel_only_shows_the_data_supplied_using_datanames
shinytest2-modules 💚 $10.32$ $-10.30$ e2e_filter_panel_shows_all_the_datasets_when_datanames_is_all
shinytest2-modules 💚 $9.64$ $-9.63$ e2e_the_module_server_logic_is_only_triggered_when_the_teal_module_becomes_active
shinytest2-reporter 💚 $18.03$ $-18.02$ e2e_adding_a_report_card_in_a_module_adds_it_in_the_report_previewer_tab
shinytest2-reporter 💚 $18.42$ $-18.41$ e2e_reporter_does_not_show_the_secondary_column_that_shows_filter_panel
shinytest2-reporter 💚 $13.47$ $-13.46$ e2e_reporter_previewer_module_do_not_show_data_summary_nor_filter_panel
shinytest2-reporter 💚 $8.28$ $-8.27$ e2e_reporter_tab_is_created_when_a_module_has_reporter
shinytest2-reporter 💚 $8.09$ $-8.08$ e2e_reporter_tab_is_not_created_when_a_module_has_no_reporter
shinytest2-show-rcode 💚 $10.55$ $-10.54$ e2e_teal_app_initializes_with_Show_R_Code_modal
shinytest2-teal_data_module 💚 $8.30$ $-8.29$ e2e_teal_data_module_gets_removed_after_successful_data_load_when_once_TRUE
shinytest2-teal_data_module 💚 $11.76$ $-11.74$ e2e_teal_data_module_inputs_change_teal_data_object_that_is_passed_to_teal_main_UI
shinytest2-teal_data_module 💚 $7.61$ $-7.60$ e2e_teal_data_module_is_still_visible_after_successful_data_load_when_once_FALSE
shinytest2-teal_data_module 💚 $5.33$ $-5.32$ e2e_teal_data_module_shows_validation_errors
shinytest2-teal_data_module 💚 $8.95$ $-8.94$ e2e_teal_data_module_will_have_a_delayed_load_of_datasets
shinytest2-teal_data_module 💚 $5.72$ $-5.71$ e2e_teal_data_module_will_make_other_tabs_inactive_before_successful_data_load
shinytest2-teal_slices 💚 $23.75$ $-23.73$ e2e_teal_slices_filters_are_initialized_when_global_filters_are_created
shinytest2-teal_slices 💚 $37.44$ $-37.43$ e2e_teal_slices_filters_are_initialized_when_module_specific_filters_are_created
shinytest2-utils 💚 $10.18$ $-10.17$ e2e_show_hide_hamburger_works_as_expected
shinytest2-wunder_bar 💚 $11.01$ $-11.00$ wunder_bar_srv_clicking_filter_icon_opens_filter_manager_modal
shinytest2-wunder_bar 💚 $10.96$ $-10.95$ wunder_bar_srv_clicking_snapshot_icon_opens_snapshot_manager_modal

Results for commit 4bb3811

♻️ This comment has been updated with latest results.

@m7pr
Copy link
Contributor

m7pr commented Dec 17, 2024

@llrs-roche

I could also duplicate the documentation on the internal function srv_validate_reactive_teal_data with the renamed parameter and keep using the old parameter on srv_transform_teal_data with the documentation for that parameter copied on its documentation file.

Please do!

m7pr
m7pr reviewed Dec 17, 2024
View reviewed changes
R/dummy_functions.R Outdated Show resolved Hide resolved
m7pr
m7pr reviewed Dec 17, 2024
View reviewed changes
R/module_nested_tabs.R Outdated Show resolved Hide resolved
@gogonzo
Copy link
Contributor

gogonzo commented Dec 17, 2024

Renaming is done selectively, there are bunch of functions which still haven't been touch. This is a diagram of data processing in teal. I've ommited some calls like srv_check_module_datanames or .resolve_module_datanames. Bold boarder means exported functions. Please analyse and compare with the changes in PR - there are some gaps. init and module$server have been released for a long time and any changes there will require unanimous decision and deprecation process (probably out of the discussion)

data arg in teal

gogonzo
gogonzo reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check these functions:

  • In .call_teal_module: filtered_teal_data -> data and set checkmate::assert_class(data, "teal_data")
  • in srv_data_summary: teal_data -> data

R/module_transform_data.R Outdated Show resolved Hide resolved
@llrs-roche
Copy link
Contributor Author

As discussed on the SU, I renamed parameters back to data (on this PR the different commits replaced like this: data -> teal_data_r -> data), and renamed data_rv, filtered_teal_data and teal_data parameter to data. This simplifies the different parameters used across teal and instead of having 3 diferents parameters we will have one. This will avoid affecting the end users and issues with documentation diverging on different places.

The data parameter may refer to different classes, but developers should rely on the assertions to know which class is accepted instead of the name.

@gogonzo setting checkmate::assert_class(data, "teal_data") inside .call_tea_module caused more than 50 test failures. I'm replacing the assertion with "reactives" as this is what it seems to be expected on tests.

gogonzo
gogonzo reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gogonzo setting checkmate::assert_class(data, "teal_data") inside .call_tea_module caused more than 50 test failures. I'm replacing the assertion with "reactives" as this is what it seems to be expected on tests.

Yup, I'm glad you've figured it out. I have no further comments

R/module_init_data.R Outdated Show resolved Hide resolved
gogonzo
gogonzo reviewed Dec 18, 2024
View reviewed changes
R/module_teal.R Outdated Show resolved Hide resolved
gogonzo
gogonzo reviewed Dec 18, 2024
View reviewed changes
R/module_teal.R Outdated Show resolved Hide resolved
gogonzo
gogonzo approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@llrs-roche llrs-roche merged commit 02edb14 into main Dec 19, 2024
27 of 28 checks passed
@llrs-roche llrs-roche deleted the 1323_renames_teal_data_modules@main branch December 19, 2024 09:41
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@m7pr m7pr m7pr left review comments

@pawelru pawelru pawelru left review comments

@gogonzo gogonzo gogonzo approved these changes

Assignees

@m7pr m7pr

Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request for renames in teal_data_module
4 participants
@llrs-roche @m7pr @gogonzo @pawelru